-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#757: Settings in code repository #850
base: main
Are you sure you want to change the base?
Conversation
…/IDEasy into settings-in-code-repo
if (!Files.isDirectory(settingsPath.resolve(GitContext.GIT_FOLDER))) { | ||
try { | ||
settingsPath = settingsPath.toRealPath(); | ||
} catch (IOException e) { | ||
throw new IllegalStateException("Could not resolve settings folder to real path.", e); | ||
} | ||
} | ||
gitContext.pull(settingsPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to pull here, you need to also traverse the real path up to where the .git
directory is.
So settingsPath.getParent()
would be required after toRealPath()
(or you do a loop upwards until you have found a .git
folder).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found something pretty interesting. When the settings repository is a symlink and I execute a git command in that repository, then the git command works. Even if the repository is just a folder inside a real repository, git commands work there too. With this I think we dont need to differentiate between the two cases (symlink or not), we can directly update the settings repository.
cli/src/main/java/com/devonfw/tools/ide/commandlet/CreateCommandlet.java
Outdated
Show resolved
Hide resolved
try { | ||
repository = repository.toRealPath(); | ||
} catch (IOException e) { | ||
throw new IllegalStateException("Couldn't resolve settings to real path.", e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have duplicated this code fragment in multiple places. I have made some suggestions to improve this.
This already indicates that this should go to a central place and should be reused.
IMHO this feature is something special for our settings
and I would recommend to keep the logic out of GitContext
.
You could add a getSettingsGitRepository()
method to IdeContext
that currently would just delegate to getSettingsPath()
but could then be extended by you to do this extra logic to find the real git repo with the .git
folder.
Also you can then detect if no .git
folder was found at all to resolve #826.
IMHO you should then log an error but return null
.
In the use-case of #826 we just wanted to check for an update of the settings. If that is not possible, we could still continue regular commands. IMHO commands like --version
or --help
should not trigger online network activity after all and especially not fail because of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot to mark this as done too
cli/src/main/java/com/devonfw/tools/ide/commandlet/CreateCommandlet.java
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/commandlet/CreateCommandlet.java
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 12299589555Details
💛 - Coveralls |
} catch (IOException e) { | ||
throw new IllegalStateException("Could not update the settings repository."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I am not mistaken, this exception can only come from toRealPath()
.
It it better to do such exception handling closer to the actual operation.
That is why we abstract such things typically in FileAccess
for reuse in multiple places.
Further, you did the famous evil bug pattern:
https://github.com/devonfw/IDEasy/blob/main/documentation/coding-conventions.adoc#catching-and-handling-exceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was due to the refresh problem, can be resolved
cli/src/main/java/com/devonfw/tools/ide/commandlet/CreateCommandlet.java
Show resolved
Hide resolved
@Override | ||
protected void updateSettings() { | ||
|
||
if (codeRepositoryFlag.isTrue() && !settingsRepo.getValue().isBlank()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do ide create my-test-project
you will be asked for the settings git URL.
Now I can do ide create my-test-project --code https://github.com/myorg/myrepo.git
to use the new feature.
It seems inconsistent to me that if I call ide create my-test-project --code
that I will be asked for a settings git URL and if I provide https://github.com/myorg/myrepo.git
this will not be treated as code repo and more or less the --code
option has no effect.
If the value is blank, you should ask for the git code repo URL.
Unlike with the settings git URL there will be no default and the question should also indicate that a code repo with settings inside is requested.
fixes #757
fixes #826